Conversation
…nd preferences management
…aders and update registration response status
…e user deletion process
…l fields and error handling
…ge of auth-service
- Use select(.kind == "Deployment") to ensure yq only modifies the Deployment - Prevents accidental modification of Service resource in multi-document YAML - Add debugging step to display file contents before apply - Explicitly checkout main branch from k8s-config repo - Matches the fix applied to API Gateway workflow
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 22222403 | Triggered | Username Password | f1d30fe | test-auth-complete.sh | View secret |
| 22222402 | Triggered | Generic High Entropy Secret | 775f9db | auth-service/src/test/resources/application-test.properties | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Warning Rate limit exceeded@RandithaK has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (62)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive authentication service enhancements including email verification, JWT refresh tokens, password reset, profile management, and user preferences. The implementation addresses all issues identified in the audit report, bringing the authentication service from 58% to 100% completion.
Key Changes:
- Implements email verification system with token-based validation and configurable expiry
- Adds JWT refresh token mechanism with 7-day expiry, IP/user-agent tracking, and revocation support
- Implements password reset flow with token-based reset and email notifications
- Adds profile management endpoints for updating user information and preferences
- Introduces new entities: VerificationToken, RefreshToken, UserPreferences
Reviewed Changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| auth-service/src/main/java/com/techtorque/auth_service/AuthServiceApplication.java | Adds dotenv loading for environment variables |
| auth-service/src/main/java/com/techtorque/auth_service/service/AuthService.java | Implements email verification, refresh tokens, and password reset flows |
| auth-service/src/main/java/com/techtorque/auth_service/service/TokenService.java | New service for managing verification and refresh tokens |
| auth-service/src/main/java/com/techtorque/auth_service/service/EmailService.java | New service for sending verification, reset, and welcome emails |
| auth-service/src/main/java/com/techtorque/auth_service/service/UserService.java | Adds profile update methods and cascading delete for related entities |
| auth-service/src/main/java/com/techtorque/auth_service/service/ProfilePhotoService.java | New service for profile photo management with BLOB storage and caching |
| auth-service/src/main/java/com/techtorque/auth_service/service/PreferencesService.java | New service for managing user notification preferences |
| auth-service/src/main/java/com/techtorque/auth_service/entity/* | New entities for tokens, preferences, and extended User fields |
| auth-service/src/main/java/com/techtorque/auth_service/controller/* | New endpoints for verification, refresh, profile, and preferences |
| auth-service/src/main/java/com/techtorque/auth_service/config/* | New configuration for email, cache, CORS, and gateway headers |
| auth-service/pom.xml | Adds dependencies for email, dotenv, and Guava caching |
| test-auth-complete.sh | Comprehensive test script for all authentication endpoints |
| README.md, IMPLEMENTATION_SUMMARY.md, COMPLETE_IMPLEMENTATION_REPORT.md | Updated documentation |
Files not reviewed (1)
- .idea/workspace.xml: Language not supported
Comments suppressed due to low confidence (1)
auth-service/src/main/java/com/techtorque/auth_service/entity/User.java:91
- getRoles exposes the internal representation stored in field roles. The value may be modified after this call to getRoles.
getRoles exposes the internal representation stored in field roles. The value may be modified after this call to getRoles.
getRoles exposes the internal representation stored in field roles. The value may be modified after this call to getRoles.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public static void main(String[] args) { | ||
| // Load environment variables from .env file | ||
| Dotenv dotenv = Dotenv.load(); |
There was a problem hiding this comment.
The dotenv loading will fail in production environments where the .env file doesn't exist. Wrap this in a try-catch block or use Dotenv.configure().ignoreIfMissing().load() to prevent application startup failures when the .env file is not present.
| Dotenv dotenv = Dotenv.load(); | |
| Dotenv dotenv = Dotenv.configure().ignoreIfMissing().load(); |
|
|
||
| User user = User.builder() | ||
| .username(registerRequest.getUsername()) | ||
| .username(registerRequest.getEmail()) // Use email as username for simplicity |
There was a problem hiding this comment.
[nitpick] Using email as username creates inconsistency since the RegisterRequest no longer has a username field, but existing code still references username. This could cause confusion when debugging user-related issues. Consider documenting this design decision clearly or adding a dedicated username field in the User entity that is displayed separately from the authentication email.
| .enabled(true) // Allow login without email verification | ||
| .emailVerified(false) // Track verification status separately |
There was a problem hiding this comment.
Setting enabled=true before email verification creates a security gap. Users can log in before verifying their email, which contradicts the comment on line 177 in the registerUser method and the design intent. The login flow should check emailVerified status and reject unverified users.
| verificationTokenRepository.findByUserAndTokenType(user, VerificationToken.TokenType.EMAIL_VERIFICATION) | ||
| .ifPresent(verificationTokenRepository::delete); |
There was a problem hiding this comment.
[nitpick] Deleting existing tokens before creating new ones requires two database operations. Consider using an upsert pattern or updating the existing token if present to reduce database round trips, especially under high registration load.
| if (!base64Image.matches("^[A-Za-z0-9+/]*={0,2}$")) { | ||
| throw new IllegalArgumentException("Base64 string contains invalid characters"); | ||
| } |
There was a problem hiding this comment.
The regex validation on line 94 could be expensive for large base64 strings (up to 5MB). Since Base64.getDecoder().decode() already throws IllegalArgumentException for invalid base64, this validation is redundant and impacts performance. Consider removing it or only using it for smaller strings.
| @RestController | ||
| // Class-level request mapping removed — endpoints are exposed as internal paths | ||
| // @RequestMapping("/api/v1/users") | ||
| @RequestMapping("/users") |
There was a problem hiding this comment.
[nitpick] The UserController now uses '/users' mapping but the SecurityConfig permits patterns like '/api/v1/auth/**'. This creates an inconsistent API structure where some endpoints are under /users (profile, preferences) while authentication endpoints are under /auth. Consider using a consistent base path like '/api/v1/users' or documenting the rationale for the split.
| @RequestMapping("/users") | |
| @RequestMapping("/api/v1/users") |
| package com.techtorque.auth_service.config; | ||
|
|
||
| import jakarta.servlet.*; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.core.Ordered; | ||
| import org.springframework.core.annotation.Order; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Custom CORS filter that ensures CORS headers are added to ALL responses, | ||
| * including redirects and error responses. | ||
| * | ||
| * This filter runs at the servlet level (before Spring Security) with high priority | ||
| * to ensure CORS headers are included on every response regardless of what happens downstream. | ||
| * | ||
| * NOTE: This filter is DISABLED because CORS is handled centrally by the API Gateway. | ||
| * The API Gateway applies CORS headers to all responses, so backend services should not | ||
| * add CORS headers to avoid duplication. | ||
| */ | ||
| @Component | ||
| @Order(Ordered.HIGHEST_PRECEDENCE) | ||
| public class CorsFilter implements Filter { | ||
|
|
||
| @Value("${app.cors.allowed-origins:http://localhost:3000,http://127.0.0.1:3000}") | ||
| private String allowedOrigins; | ||
|
|
||
| @Override | ||
| public void init(FilterConfig filterConfig) { | ||
| // Initialize filter | ||
| } | ||
|
|
||
| @Override | ||
| public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) | ||
| throws IOException, ServletException { | ||
|
|
||
| // CORS is handled by the API Gateway, so we skip CORS header injection here | ||
| // Just pass the request through without adding CORS headers | ||
| chain.doFilter(request, response); | ||
| } | ||
|
|
||
| @Override | ||
| public void destroy() { | ||
| // Cleanup | ||
| } | ||
|
|
||
| /** | ||
| * Check if the given origin is in the allowed list | ||
| */ | ||
| private boolean isOriginAllowed(String origin) { | ||
| String[] origins = allowedOrigins.split(","); | ||
| for (String allowed : origins) { | ||
| if (allowed.trim().equals(origin)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The CorsFilter component is registered but does nothing (effectively a no-op). This adds unnecessary overhead to every request. If CORS is handled by the API Gateway, remove this filter component entirely rather than leaving it as dead code.
| package com.techtorque.auth_service.config; | |
| import jakarta.servlet.*; | |
| import jakarta.servlet.http.HttpServletResponse; | |
| import jakarta.servlet.http.HttpServletRequest; | |
| import org.springframework.beans.factory.annotation.Value; | |
| import org.springframework.core.Ordered; | |
| import org.springframework.core.annotation.Order; | |
| import org.springframework.stereotype.Component; | |
| import java.io.IOException; | |
| /** | |
| * Custom CORS filter that ensures CORS headers are added to ALL responses, | |
| * including redirects and error responses. | |
| * | |
| * This filter runs at the servlet level (before Spring Security) with high priority | |
| * to ensure CORS headers are included on every response regardless of what happens downstream. | |
| * | |
| * NOTE: This filter is DISABLED because CORS is handled centrally by the API Gateway. | |
| * The API Gateway applies CORS headers to all responses, so backend services should not | |
| * add CORS headers to avoid duplication. | |
| */ | |
| @Component | |
| @Order(Ordered.HIGHEST_PRECEDENCE) | |
| public class CorsFilter implements Filter { | |
| @Value("${app.cors.allowed-origins:http://localhost:3000,http://127.0.0.1:3000}") | |
| private String allowedOrigins; | |
| @Override | |
| public void init(FilterConfig filterConfig) { | |
| // Initialize filter | |
| } | |
| @Override | |
| public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) | |
| throws IOException, ServletException { | |
| // CORS is handled by the API Gateway, so we skip CORS header injection here | |
| // Just pass the request through without adding CORS headers | |
| chain.doFilter(request, response); | |
| } | |
| @Override | |
| public void destroy() { | |
| // Cleanup | |
| } | |
| /** | |
| * Check if the given origin is in the allowed list | |
| */ | |
| private boolean isOriginAllowed(String origin) { | |
| String[] origins = allowedOrigins.split(","); | |
| for (String allowed : origins) { | |
| if (allowed.trim().equals(origin)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| } |
| <dependency> | ||
| <groupId>com.google.guava</groupId> | ||
| <artifactId>guava</artifactId> | ||
| <version>32.1.3-jre</version> |
There was a problem hiding this comment.
[nitpick] Guava version 32.1.3-jre is relatively old (released in 2023). Consider using a more recent version like 33.x to benefit from bug fixes and security patches. Check the Guava release notes for any breaking changes before upgrading.
| <version>32.1.3-jre</version> | |
| <version>33.1.0-jre</version> |
| /** | ||
| * Get profile photo for any user by username | ||
| * - Returns base64 encoded image data | ||
| * - Publicly accessible for user profiles | ||
| * - Returns null if no photo exists | ||
| * | ||
| * @param username The username to get photo for | ||
| * @return ProfilePhotoDto with base64 encoded image | ||
| */ | ||
| @GetMapping("/username/{username}") | ||
| @Operation(summary = "Get user profile photo by username", description = "Retrieve profile photo for a specific user") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "Photo retrieved successfully"), | ||
| @ApiResponse(responseCode = "204", description = "No photo found") | ||
| }) | ||
| public ResponseEntity<?> getProfilePhotoByUsername(@PathVariable String username) { | ||
| // This would require a UserRepository method to find user by username | ||
| // For now, returning 204 (No Content) as placeholder | ||
| // TODO: Implement once UserRepository is extended | ||
| return ResponseEntity.noContent().build(); | ||
| } |
There was a problem hiding this comment.
The getProfilePhotoByUsername endpoint is documented but returns a placeholder implementation. This incomplete endpoint should either be fully implemented or removed until it can be properly developed to avoid confusion and potential misuse in production.
| /** | |
| * Get profile photo for any user by username | |
| * - Returns base64 encoded image data | |
| * - Publicly accessible for user profiles | |
| * - Returns null if no photo exists | |
| * | |
| * @param username The username to get photo for | |
| * @return ProfilePhotoDto with base64 encoded image | |
| */ | |
| @GetMapping("/username/{username}") | |
| @Operation(summary = "Get user profile photo by username", description = "Retrieve profile photo for a specific user") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = "Photo retrieved successfully"), | |
| @ApiResponse(responseCode = "204", description = "No photo found") | |
| }) | |
| public ResponseEntity<?> getProfilePhotoByUsername(@PathVariable String username) { | |
| // This would require a UserRepository method to find user by username | |
| // For now, returning 204 (No Content) as placeholder | |
| // TODO: Implement once UserRepository is extended | |
| return ResponseEntity.noContent().build(); | |
| } | |
| // /** | |
| // * Get profile photo for any user by username | |
| // * - Returns base64 encoded image data | |
| // * - Publicly accessible for user profiles | |
| // * - Returns null if no photo exists | |
| // * | |
| // * @param username The username to get photo for | |
| // * @return ProfilePhotoDto with base64 encoded image | |
| // */ | |
| // @GetMapping("/username/{username}") | |
| // @Operation(summary = "Get user profile photo by username", description = "Retrieve profile photo for a specific user") | |
| // @ApiResponses(value = { | |
| // @ApiResponse(responseCode = "200", description = "Photo retrieved successfully"), | |
| // @ApiResponse(responseCode = "204", description = "No photo found") | |
| // }) | |
| // public ResponseEntity<?> getProfilePhotoByUsername(@PathVariable String username) { | |
| // // This would require a UserRepository method to find user by username | |
| // // For now, returning 204 (No Content) as placeholder | |
| // // TODO: Implement once UserRepository is extended | |
| // return ResponseEntity.noContent().build(); | |
| // } |
No description provided.